stunnel 5.61 support #4807

julek-wolfssl · 2022-01-28T15:41:44Z

New/Implemented API
- SSL_has_pending
- wolfSSL_CertManagerLoadCRLFile
- wolfSSL_LoadCRLFile
- wolfSSL_CTX_LoadCRLFile
- wolfSSL_CTX_add_session
Access to session cache is now atomic
- Adding and getting sessions to and from the local cache is now atomic.
- The new internal wolfSSL_GetSessionFromCache requires a destination object to be supplied when retrieving from the cache so that items can be retrieved independently from the cache. For most existing calls, the destination is ssl->session.
- PREALLOC_SESSION_TICKET_LEN defines how much memory is temporarily allocated for the ticket if it doesn't fit in the static session buffer.
- Before this pull request, wolfSSL_get_session always returned a pointer to the internal session cache. The user can't tell if the underlying session hasn't changed before it calls wolfSSL_set_session on it. This PR adds a define NO_SESSION_CACHE_REF (for now only defined with OPENSSL_COMPATIBLE_DEFAULTS) that makes wolfSSL only return a pointer to ssl->session. The issue is that this makes the pointer returned non-persistent ie: it gets free'd with the WOLFSSL object. This pull request leverages the lightweight ClientCache to "increase" the size of the session cache. The hash of the session ID is checked to make sure that the underlying session hasn't changed.
Calling chain certificate API (for example wolfSSL_CTX_use_certificate_chain_file) no longer requires an actual chain certificate PEM file to be passed in as input. ProcessUserChain error in ProcessBuffer is ignored if it returns that it didn't find a chain.
Add WOLFSSL_TICKET_HAVE_ID macro. When defined tickets will include the original session ID that can be used to lookup the session in internal cache. This is useful for fetching information about the peer that doesn't get sent in a resumption (such as the peer's certificate chain).
- Add ssl->session->altSessionID field because ssl->session.sessionID is used to return the "bogus" session ID sent by the client in TLS 1.3 and when using tickets in TLS <= 1.2
OPENSSL_COMPATIBLE_DEFAULTS changes
- Define WOLFSSL_TRUST_PEER_CERT and certificates added as CA's will also be loaded as trusted peer certificates
Seperate internalCacheOff and internalCacheLookupOff options to govern session addition and lookup
VerifyServerSuite now determines if RSA is available by checking for it directly and not assuming it as the default if static ECC is not available
WOLFSSL_SESSION changes
- ssl->session->altSessionID added to return a dynamic session when internalCacheOff is set
- ssl->session.refPtr removed in favor of making ssl->session a seperate heap allocated object that can be returned to the user
If SSL_MODE_AUTO_RETRY is set then retry should only occur during a handshake with a limit
WOLFSSL_TRUST_PEER_CERT code now always uses cert->subjectHash for the cm->tpTable table row selection
Change some error message names to line up with OpenSSL equivalents
Run certificate setup callback before MatchSuite
Refactor clearing ASN_NO_PEM_HEADER off the error queue into a macro
wolfSSL_get_peer_certificate now returns a duplicated object meaning that the caller needs to free the returned object
Align wolfSSL_CRYPTO_set_mem_functions callbacks with OpenSSL API
wolfSSL_d2i_PKCS12_bio now consumes the input BIO. It now supports all supported BIO's instead of only memory BIO.
stunnel specific
- When allocating a dynamic session, always do wolfSSL_SESSION_set_ex_data(session, 0, (void *)(-1). This is to mimic the new index callback set in SSL_SESSION_get_ex_new_index.
Fix comment in wolfSSL_AES_cbc_encrypt
Trusted peer certificate suite tests need to have CRL disabled since we don't have the issuer certificate in the CA store if the certificates are only added as trusted peer certificates.

dgarske

Great work. A few minor items to review.

src/internal.c

dgarske · 2022-01-28T17:33:54Z

src/ssl.c

-            *pkcs12 = NULL;
+    if (mem != NULL && localPkcs12 != NULL) {
+        if (wolfSSL_BIO_read(bio, mem, (int)memSz) == memSz) {
+            ret = wc_d2i_PKCS12(mem, memSz, localPkcs12);


src/ssl.c:63445:38: error: implicit conversion loses integer precision: 'long' to 'word32' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32] ret = wc_d2i_PKCS12(mem, memSz, localPkcs12); ~~~~~~~~~~~~~ ^~~~~

Fixed as part of the Jenkins fixes.

wolfcrypt/src/error.c

src/ssl.c

julek-wolfssl · 2022-01-28T22:45:42Z

Also need to add new session fields to i2d and d2i api.
Edit: Actually not needed. No new fields to export/import.

dgarske

I think there is great work in this PR, but we need to be very careful about how this will impact our customers... memory use, locking performance, API compatibility, etc...

Will you look into the resume test failures? Thanks

src/ssl.c

dgarske · 2022-02-02T20:27:00Z

src/ssl.c

@@ -12459,7 +12460,7 @@ WOLFSSL_SESSION* wolfSSL_get_session(WOLFSSL* ssl)
 {
    WOLFSSL_ENTER("SSL_get_session");
    if (ssl)
-        return wolfSSL_GetSession(ssl, NULL, 1);
+        return &ssl->session;


This doesn't work. wolfSSL_get_session is called... WOLFSSL object can be free'd between resumption.

That is correct because wolfSSL_get_session should return a reference to the working session (that is what SSL_get_session does). Users who want to retain a copy of the session should always use wolfSSL_get1_session as that will guarantee either an incremented refCount of a duplicated object.

This will break existing compatibility. Did you consider making the WOLFSSL_SESSION in WOLFSSL a pointer and allocating from heap? It would not increase memory use, since the WOLFSSL object is already allocated from the heap.

This will break existing compatibility.

While true, the assumption is that a pointer directly to the cache entry is just as unsafe. The client user has no way of knowing if the session didn't change before setting it in a new SSL object. We could make wolfSSL_get_session and wolfSSL_get1_session do the same thing but that will cause leaks since wolfSSL_get_session is meant to return a reference to the SSL object's working session.

Did you consider making the WOLFSSL_SESSION in WOLFSSL a pointer and allocating from heap?

This is an interesting idea. It should be fairly easy to do. The only issue would be that the heap is further fragmented. Do you think I should pursue this direction?

Yes please. You'll have to track that the allocated WOLFSSL_SESSION was retrieved and assume to not free it on wolfSSL_free. I think the session_ref stuff already does something similar, which could all be cleaned up and combined.

Do you mean that the session should point into the cache? I was thinking it would be allocated on wolfSSL_new and free'd on wolfSSL_free. A user could retain the object by either calling wolfSSL_get1_session or the up_ref API.

No, allocated.

Got it. I'll try to get it done tomorrow.

julek-wolfssl · 2022-02-03T15:23:28Z

memory use, locking performance, API compatibility

Out of these three, I think API compatibility is the only concern. Memory use will not be increased because ssl->session destination is always allocated anyway. The only increase in memory would be the temporary ticket buffer allocated either on the stack or the heap which gets freed immediately after we are done with cache access. Locking performance should be better since we will only lock once per access with no system calls inside.

dgarske

Great work on this! Lots to review here. First pass feedback.

dgarske · 2022-02-11T18:39:09Z

src/internal.c

@@ -6683,10 +6683,10 @@ int InitSSL(WOLFSSL* ssl, WOLFSSL_CTX* ctx, int writeDup)
 void FreeArrays(WOLFSSL* ssl, int keep)
 {
    if (ssl->arrays) {
-        if (keep) {
+        if (keep && !IsAtLeastTLSv1_3(ssl->version)) {


Since ssl->session is a pointer now, please also add NULL check before trying to touch it.

configure.ac

dgarske · 2022-02-11T18:42:49Z

src/internal.c

@@ -8983,7 +8972,8 @@ static int wolfSSLReceive(WOLFSSL* ssl, byte* buf, word32 sz)
                return -1;

            case WOLFSSL_CBIO_ERR_WANT_READ:      /* want read, would block */
-                if (ssl->ctx->autoRetry)
+                if (ssl->ctx->autoRetry && !ssl->options.handShakeDone &&


Good fix here... This is probably one that needs to be merged soon.

dgarske · 2022-02-11T18:43:53Z

src/internal.c

@@ -20201,7 +20193,11 @@ const char* wolfSSL_ERR_reason_error_string(unsigned long e)
        return "wrong client/server type";

    case NO_PEER_CERT :
+#ifndef OPENSSL_EXTRA


It is fine with me if you change NO_PEER_CERT and BAD_BINDER for all build cases (not just OPENSSL_EXTRA). But consider adding comment that openssl compatibility expects this specific text.

One fixed value with comment to explain compatibility.

dgarske · 2022-02-11T18:47:12Z

src/internal.c

@@ -29955,6 +29942,15 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
 #endif

        ret = MatchSuite(ssl, &clSuites);
+#ifdef OPENSSL_EXTRA
+        /* Give user last chance to provide a cert for cipher selection */


This section is a very good change. If someone has setup a certificate callback with wolfSSL_CTX_set_cert_cb then they have the option to load different certificate and the call below to try MatchSuite again will adjust.

I'd prefer this logic to be cleaned up and only called if ssl->ctx->certSetupCb is set. I do realize it is also checked in CertSetupCbWrapper, but the return code logic from it doesn't report error for it. I want to make sure a future change doesn't cause an issue here.

Also does ret = MatchSuite(ssl, &clSuites); above at 29944 still need called before or would it make sense to just always call after the CertSetupCbWrapper?

I was thinking that the callback could be dependent on the cipher chosen but that doesn't make sense since the certificate selection will heavily influence the ciphersuite. Changing to one call after the cert callback.

dgarske · 2022-02-11T18:49:12Z

src/internal.c

@@ -30513,6 +30504,9 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
    #ifdef WOLFSSL_EARLY_DATA
        word32          maxEarlyDataSz;        /* Max size of early data */
    #endif
+#endif
+#ifdef WOLFSSL_TICKET_HAVE_ID
+        byte            id[ID_LEN];


Added 32 bytes here should be fine, since the total ticket size is still under the SESSION_TICKET_LEN=256 default.

So do you think this should be added permanently for all configurations with the session cache enabled?

src/ssl.c

dgarske · 2022-02-11T18:58:18Z

src/internal.c

-        XFREE(ssl->session.ticket, ssl->heap, DYNAMIC_TYPE_SESSION_TICK);
-        ssl->session.ticket = ssl->session._staticTicket;
-        ssl->session.ticketLenAlloc = 0;
+    if (ssl->session->ticketLenAlloc > 0) {


Please add a ssl->session != NULL check to this function. I simulated free'ing the ssl->session in FreeHandshakeResources as a test...

The ssl->session member is allocated when the WOLFSSL object is created and free'ed only on wolfSSL_free. It is used extensively everywhere. Other members aren't tested for either like ssl->arrays or ssl->suites. I'm not sure if this is really necessary. Are you sure?

$ grep -r -P --include='*.[ch]' '\-\>session\-\>' | wc -l 233

dgarske · 2022-02-11T18:58:58Z

src/tls13.c

-    ssl->session.cipherSuite    = ssl->options.cipherSuite;
-    ssl->session.ticketSeen     = now;
-    ssl->session.ticketAdd      = ageAdd;
+    ssl->session->timeout        = lifetime;


Same.. please add an ssl->session != NULL check in DoTls13NewSessionTicket

dgarske · 2022-02-11T19:12:12Z

src/ssl.c

-        session->sessionIDSz = ID_LEN;
+    (void)error;
+
+    WOLFSSL_ENTER("AddSession");


Please add check for ssl->session == NULL.

julek-wolfssl · 2022-02-16T09:30:52Z

Jenkins failure due to aborted test.

dgarske

Lots of excellent work in this PR and looking forward to getting it merged. Just a few minor feedback items.

dgarske · 2022-02-22T23:50:05Z

tests/api.c

+            test_wolfSSL_CTX_add_session_server_ctx == NULL) {
+        AssertNotNull(test_wolfSSL_CTX_add_session_server_ctx
+                = wolfSSL_get_SSL_CTX(ssl));
+        AssertIntEQ(wolfSSL_CTX_up_ref(wolfSSL_get_SSL_CTX(ssl)),


Building with ./configure CFLAGS="-DWOLFSSL_TICKET_HAVE_ID -DHAVE_EXT_CACHE" && make gives error because wolfSSL_CTX_up_ref is not available. You might consider making that API public when session cache is enabled, not just on compatibility layer?

I made wolfSSL_CTX_up_ref always available. As I explain in the commit message:

wolfSSL_CTX_up_ref is a small and potentially useful API for users so it doesn't need to be restricted only to the compatibility layer. The reference counting mechanisms are always available anyway. This just exposes the functionality to the user.

src/ssl.c

- New/Implemented API - `SSL_has_pending` - `wolfSSL_CertManagerLoadCRLFile` - `wolfSSL_LoadCRLFile` - `wolfSSL_CTX_LoadCRLFile` - `wolfSSL_CTX_add_session` - Calling chain certificate API (for example `wolfSSL_CTX_use_certificate_chain_file`) no longer requires an actual chain certificate PEM file to be passed in as input. `ProcessUserChain` error in `ProcessBuffer` is ignored if it returns that it didn't find a chain. - Add `WOLFSSL_TICKET_HAVE_ID` macro. When defined tickets will include the original session ID that can be used to lookup the session in internal cache. This is useful for fetching information about the peer that doesn't get sent in a resumption (such as the peer's certificate chain). - Add `ssl->ticketSessionID` field because `ssl->session.sessionID` is used to return the "bogus" session ID sent by the client in TLS 1.3 - `OPENSSL_COMPATIBLE_DEFAULTS` changes - Define `WOLFSSL_TRUST_PEER_CERT` and certificates added as CA's will also be loaded as trusted peer certificates - Define `WOLFSSL_TLS13_MIDDLEBOX_COMPAT` - Seperate `internalCacheOff` and `internalCacheLookupOff` options to govern session addition and lookup - `VerifyServerSuite` now determines if RSA is available by checking for it directly and not assuming it as the default if static ECC is not available - `WOLFSSL_SESSION` changes - `ssl->extSession` added to return a dynamic session when internalCacheOff is set - `ssl->session.refPtr` made dynamic and gets free'd in `SSL_ResourceFree` - If `SSL_MODE_AUTO_RETRY` is set then retry should only occur during a handshake - `WOLFSSL_TRUST_PEER_CERT` code now always uses `cert->subjectHash` for the `cm->tpTable` table row selection - Change some error message names to line up with OpenSSL equivalents - Run `MatchSuite` again if certificate setup callback installed and successful - Refactor clearing `ASN_NO_PEM_HEADER` off the error queue into a macro - `wolfSSL_get_peer_certificate` now returns a duplicated object meaning that the caller needs to free the returned object - Allign `wolfSSL_CRYPTO_set_mem_functions` callbacks with OpenSSL API - `wolfSSL_d2i_PKCS12_bio` now consumes the input BIO. It now supports all supported BIO's instead of only memory BIO. - stunnel specific - Always return a session object even if we don't have a session in cache. This allows stunnel to save information in the session external data that will be transfered to new connections if the session is reused - When allocating a dynamic session, always do `wolfSSL_SESSION_set_ex_data(session, 0, (void *)(-1)`. This is to mimic the new index callback set in `SSL_SESSION_get_ex_new_index`. - Fix comment in `wolfSSL_AES_cbc_encrypt` - Trusted peer certificate suite tests need to have CRL disabled since we don't have the issuer certificate in the CA store if the certificates are only added as trusted peer certificates. tested

- Adding and getting sessions to and from the local cache is now atomic. - The new internal `wolfSSL_GetSessionFromCache` requires a destination object to be supplied when retrieving from the cache so that items can be retrieved independently from the cache. For most existing calls, the destination is `ssl->session`. -`PREALLOC_SESSION_TICKET_LEN` defines how much memory is temporarily allocated for the ticket if it doesn't fit in the static session buffer.

- Refactor session cache access into `AddSessionToCache` and `wolfSSL_GetSessionFromCache`

- Move test to `HAVE_IO_TESTS_DEPENDENCIES` - Implement `wolfSSL_trust_peer_cert` - have{cipher} options weren't being set with only RSA enabled

Before this pull request, `wolfSSL_get_session` always returned a pointer to the internal session cache. The user can't tell if the underlying session hasn't changed before it calls `wolfSSL_set_session` on it. This PR adds a define `NO_SESSION_CACHE_REF` (for now only defined with `OPENSSL_COMPATIBLE_DEFAULTS`) that makes wolfSSL only return a pointer to `ssl->session`. The issue is that this makes the pointer returned non-persistent ie: it gets free'd with the `WOLFSSL` object. This commit leverages the lightweight `ClientCache` to "increase" the size of the session cache. The hash of the session ID is checked to make sure that the underlying session hasn't changed.

- Make `InternalTicket` memory alignment independent

`wolfSSL_CTX_up_ref` is a small and potentially useful API for users so it doesn't need to be restricted only to the compatibility layer. The reference counting mechanisms are always available anyway. This just exposes the functionality to the user.

Reported in ZD13606

dgarske

Big changes! Session support massive refactor.

* Adds stateful handling of DH shared secret computation in `SetupKeys`. * Improved the decrypt handling to use internal functions and avoid generating alerts on failures. * Fix for sniffer resume due to missing `sessionIDSz` broken in wolfSSL#4807. * Fix sniffer test cases to split resume (session_ticket) tests. * Add `snifftest` list of build features so test script can gate running resume test.

julek-wolfssl self-assigned this Jan 28, 2022

julek-wolfssl requested a review from dgarske January 28, 2022 15:41

julek-wolfssl force-pushed the stunnel-5.61 branch from 083c95f to 6874a8d Compare January 28, 2022 16:07

dgarske requested changes Jan 28, 2022

View reviewed changes

dgarske mentioned this pull request Jan 28, 2022

Add support for stunnel 5.61 wolfSSL/osp#103

Merged

dgarske self-requested a review February 1, 2022 23:10

dgarske self-assigned this Feb 1, 2022

julek-wolfssl mentioned this pull request Feb 2, 2022

5.0.0 OpenSSL compatibility issue: blocking SSL_peek() returns SSL_ERROR_WANT_READ. #4593

Closed

dgarske requested changes Feb 2, 2022

View reviewed changes

dgarske removed their assignment Feb 2, 2022

julek-wolfssl force-pushed the stunnel-5.61 branch 13 times, most recently from e007ecc to 794fe43 Compare February 11, 2022 18:11

dgarske self-assigned this Feb 11, 2022

dgarske self-requested a review February 11, 2022 18:17

julek-wolfssl force-pushed the stunnel-5.61 branch from 794fe43 to 58ff372 Compare February 11, 2022 18:26

dgarske requested changes Feb 11, 2022

View reviewed changes

julek-wolfssl force-pushed the stunnel-5.61 branch from 58ff372 to 2ee15ea Compare February 14, 2022 18:13

julek-wolfssl requested a review from dgarske February 14, 2022 20:38

julek-wolfssl force-pushed the stunnel-5.61 branch 2 times, most recently from 13d17c6 to d01baed Compare February 15, 2022 11:25

dgarske requested changes Feb 23, 2022

View reviewed changes

dgarske removed their assignment Feb 23, 2022

julek-wolfssl added 8 commits February 23, 2022 09:47

Allocate ssl->session separately on the heap

91b08fb

- Refactor session cache access into `AddSessionToCache` and `wolfSSL_GetSessionFromCache`

Fixes for Jenkins tests

ceff401

- Move test to `HAVE_IO_TESTS_DEPENDENCIES` - Implement `wolfSSL_trust_peer_cert` - have{cipher} options weren't being set with only RSA enabled

Fix misc memory issues

617eda9

- Make `InternalTicket` memory alignment independent

Peeking can't return a WOLFSSL_ERROR_WANT_READ in compatibility mode

d1f5305

julek-wolfssl force-pushed the stunnel-5.61 branch from 4609207 to fb943a2 Compare February 23, 2022 08:56

julek-wolfssl assigned dgarske and unassigned julek-wolfssl Feb 23, 2022

Prevent possibility of an infinite retry loop and resource exhaution

2c978a9

Reported in ZD13606

dgarske approved these changes Feb 23, 2022

View reviewed changes

dgarske merged commit 0824a64 into wolfSSL:master Feb 23, 2022

dgarske added a commit to dgarske/wolfssl that referenced this pull request Feb 24, 2022

Fixes for multi-test pass. Breaks from PR wolfSSL#4807.

2b794f0

dgarske mentioned this pull request Feb 24, 2022

Fixes for minor whitespace and scan tools #4891

Merged

dgarske mentioned this pull request Mar 15, 2022

Sniffer asynchronous support #4875

Merged

4 tasks

gojimmypi pushed a commit to gojimmypi/wolfssl that referenced this pull request Apr 23, 2022

Fixes for multi-test pass. Breaks from PR wolfSSL#4807.

9d7c257

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stunnel 5.61 support #4807

stunnel 5.61 support #4807

julek-wolfssl commented Jan 28, 2022 •

edited

Loading

dgarske left a comment

dgarske Jan 28, 2022

julek-wolfssl Feb 11, 2022

julek-wolfssl commented Jan 28, 2022 •

edited

Loading

dgarske left a comment

dgarske Feb 2, 2022

julek-wolfssl Feb 2, 2022

dgarske Feb 2, 2022

julek-wolfssl Feb 3, 2022

dgarske Feb 3, 2022

julek-wolfssl Feb 3, 2022

dgarske Feb 3, 2022

julek-wolfssl Feb 3, 2022

julek-wolfssl commented Feb 3, 2022

dgarske left a comment

dgarske Feb 11, 2022

dgarske Feb 11, 2022

dgarske Feb 11, 2022

julek-wolfssl Feb 15, 2022

dgarske Feb 11, 2022

julek-wolfssl Feb 15, 2022

dgarske Feb 11, 2022

julek-wolfssl Feb 15, 2022

dgarske Feb 11, 2022

julek-wolfssl Feb 11, 2022

dgarske Feb 11, 2022

dgarske Feb 11, 2022

julek-wolfssl commented Feb 16, 2022

dgarske left a comment

dgarske Feb 22, 2022

julek-wolfssl Feb 23, 2022

dgarske left a comment

stunnel 5.61 support #4807

stunnel 5.61 support #4807

Conversation

julek-wolfssl commented Jan 28, 2022 • edited Loading

dgarske left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julek-wolfssl commented Jan 28, 2022 • edited Loading

dgarske left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julek-wolfssl commented Feb 3, 2022

dgarske left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julek-wolfssl commented Feb 16, 2022

dgarske left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgarske left a comment

Choose a reason for hiding this comment

julek-wolfssl commented Jan 28, 2022 •

edited

Loading

julek-wolfssl commented Jan 28, 2022 •

edited

Loading